Skip to content

fix(convert): prevent argv out-of-bounds write and use PID-specific workspace#842

Closed
aki1770-del wants to merge 1 commit into
COVESA:masterfrom
aki1770-del:fix/convert-argv-overflow-793
Closed

fix(convert): prevent argv out-of-bounds write and use PID-specific workspace#842
aki1770-del wants to merge 1 commit into
COVESA:masterfrom
aki1770-del:fix/convert-argv-overflow-793

Conversation

@aki1770-del

@aki1770-del aki1770-del commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Problem

dlt-convert -t <tarball> has two security issues:

1. argv out-of-bounds write (CWE-131 / stack corruption)

After extracting files to the workspace directory, the code overwrites argc:

n = scandir(DLT_CONVERT_WS, &files, NULL, alphasort);
argc = optind + (n - 2);   // argc extended based on files in /tmp

A subsequent loop then writes to argv[index] using the inflated argc:

for (index = optind; index < argc; index++) {
    ...
    argv[index] = tmp_filename;   // index may exceed original argc!
}

If a malicious local user pre-populates /tmp/dlt_convert_workspace/ with extra files before execution, n is inflated, argc is set beyond the original argv array bounds, and argv[index] writes corrupt the stack — leading to a stack overflow as demonstrated with AddressSanitizer in #793.

2. Predictable workspace path (CWE-377)

The fixed path /tmp/dlt_convert_workspace/ is world-writable and predictable. Any local user can pre-create it with arbitrary contents before dlt-convert is run, triggering the overflow or injecting unexpected .dlt files.

Fixes #793.

Fix

For (1): Introduce a current_file pointer. In the processing loop, set it to tmp_filename in tarball mode or argv[index] otherwise. Remove argv[index] = tmp_filename entirely — the original argv array is never written to.

For (2): Use a PID-specific workspace path /tmp/dlt_convert_ws_<pid>/ (format string DLT_CONVERT_WS_FMT). The path is unique per invocation and not guessable by other local processes.

Testing

@aki1770-del
fix(convert): prevent argv out-of-bounds write and use PID-specific w…
23cd85a 
…orkspace

Two security issues in dlt-convert's -t (tarball) mode:

1. ARGV OUT-OF-BOUNDS WRITE (CVE-class: CWE-131)
   After extracting files to the workspace, argc was overwritten:
     argc = optind + (n - 2);   // n from scandir of /tmp workspace
   A subsequent loop then wrote:
     argv[index] = tmp_filename; // index may exceed original argc
   If an attacker pre-populated /tmp/dlt_convert_workspace/ with
   extra files before execution, 'n' would be inflated, argc
   extended beyond the original argv array bounds, and subsequent
   argv[index] writes would corrupt the stack.

   Fix: introduce 'current_file' and set it to tmp_filename (tflag)
   or argv[index] (no tflag) without ever writing back to argv[].

2. PREDICTABLE WORKSPACE PATH (CWE-377)
   The fixed path /tmp/dlt_convert_workspace/ allowed any local user
   to pre-create the directory with arbitrary files before execution,
   triggering the overflow above or injecting unexpected .dlt files.

   Fix: use a PID-specific path /tmp/dlt_convert_ws_<pid>/ which
   is unique per invocation and not guessable by other processes.

Fixes COVESA#793
@aki1770-del

aki1770-del commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Closing this one.

I opened a batch of PRs on this repo over a few days in early April without first doing the build-and-test verification a change in this codebase deserves, and they have been sitting in your queue since. Holding a review slot I didn't earn isn't fair to you, so I'm withdrawing them rather than leaving them open.

The branch stays up. If any of the diff is useful, please take it freely — no attribution needed, and no need to reply here.

Sorry for the noise.

@fizzl

fizzl commented Jul 12, 2026 via email •
edited
Loading

Copy link
Copy Markdown

@aki1770-del

aki1770-del commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

"Inflated argc" and "writes to argv[index]" connect through the argc reassignment.

In the tar path argc is overwritten away from its real meaning: argc = optind + (n - 2), where n is scandir's count of the workspace directory. argc no longer bounds argv — it now counts unpacked files. If the archive holds more members than there were command-line arguments, argc exceeds argv's real length. That's the "inflated" part.

Then for (index = optind; index < argc; index++) runs to that inflated bound, and inside it argv[index] = tmp_filename is a write. Once index passes the real argument count, that write lands past the end of argv. So: scandir inflates argc → the loop bound is now larger than argv → argv[index] = ... goes out of bounds. That's how the three phrases relate.

The underlying issue is reusing argc/argv as scratch storage for the unpacked-file list; the clean fix is a separate count and array, not patching the bound in place. That part isn't mine to write — I don't know this codebase well enough — and it's your call whether it warrants its own issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] stack-overflow in dlt-convert.c

2 participants

@aki1770-del @fizzl